Intent Analysis

Problem: The hosted CI XCTest runner (Scripts/ci_app_test_runner.py) is strictly serial; the repo wants to canary process-level suite sharding later without changing default CI behavior now.

Before → After: Before: suites run one group at a time with no shared-state classification. After: new --ledger, --serial-group-policy, --workers (default 1), and --print-suite-plan-json; ledger rows aggregate into suite tags/tiers/runtime estimates; suites classify as pinned_serial vs parallel_eligible via the new Scripts/Fixtures/ci-test-serial-groups.json; with --workers > 1, eligible groups run in a thread pool with fail-fast cancellation while pinned groups run serially on the coordinator thread; targeted teardown added to five known shared-state test files. Hosted CI behavior is unchanged: the workflow diff contains only actions/cache@v4→v6 — no --workers flag is passed anywhere, so the parallel path is genuinely dormant.

Evidence: Verified from the head commit (c131e74) directly, not the PR description: --workers defaults to 1 (ci_app_test_runner.py:1394–1398); run_all_suites takes the pre-existing serial loop when workers == 1 (:1230–1246); test_workers_one_preserves_current_serial_execution_order pins that; .github/workflows/ci.yml diff is the cache bump only.

Confidence: High on intent and dormancy. Medium on runtime-estimate metadata accuracy (no linked perf evidence; it only affects planning of an opt-in mode).

Red Flags: (1) A confirmed cancellation-attribution defect in the new workers>1 scheduler (below). (2) The "fail-fast behavior" self-test mocks run_suite_buffered entirely, so the novel cancellation interaction is never exercised. (3) An unrelated cache-action bump is bundled in.

Findings

F1 (blocking, Medium): cancelled peer can be recorded as first failure, corrupting the exit code. Confirmed mechanism by code inspection: run_suite_buffered sets cancellation_event on the worker thread when its result is neither passed nor cancelled (:1023–1027); a peer observing the event returns state="cancelled", exit_code=130 (:748–762); the coordinator processes completed futures sorted by group label (:1314) and treats any non-passed result as first_failure (:1320–1328) — missing the state == "cancelled" → continue guard the pinned branch has at :1300. Because pinned groups run inline on the coordinator thread (:1282–1307), the real failure and a cancelled peer can land in one done batch, and a peer whose label sorts first yields exit 130 and misattributed first failure. Impact is bounded: fail-fast still fails the run (no false pass) and the real failure's buffered output still prints (:1319) — this is exit-code/attribution corruption, not broken fail-fast. A first-pass reviewer reported ~50/100 reproduction; I did not independently reproduce that frequency, but the mechanism does not depend on it. The existing self-test (test_ci_app_test_runner.py:969–1019) mocks run_suite_buffered with a fake that never sets the event or returns cancelled, so it cannot catch this. Fix: add the cancelled-skip guard in the futures branch (safe — a cancelled result implies the real failed future already resolved and cannot be .cancel()ed) plus one self-test whose fake returns a cancelled peer alongside a failure.

F2 (non-blocking, Low): suites missing from the ledger classify parallel_eligible outside --strict-ledger (:312–326, :1063–1081). Inert at workers=1; any future canary invocation should pass --strict-ledger.

F3 (informational): pinned groups on the coordinator thread mean effective concurrency is workers + 1, and pinned suites overlap the worker pool — defensible if lane tags fully partition conflicts; worth a comment before the canary.

F4 (informational): the actions/cache@v6 bump is unrelated to sharding prep; fine to keep, but call it out.

Maintainer-guidance check

Cause-fixing: F1's fix addresses the missing cancelled-state handling, not a symptom. One authority: ledger stays the single tag authority; the policy fixture is a separate, strictly validated input. State safety: teardowns are targeted; no user-state surface touched. Cancellation/partial-success: the guidance explicitly requires checking cancellation behavior before declaring a design safe — F1 is that check failing. Observability: exit code and first-failure attribution are the runner's contract; F1 damages them in the new mode. Boundary-appropriate validation: the affected boundary is the workers>1 scheduler, and the shipped self-tests mock past its one novel racy interaction, so the validation claim is materially incomplete.

Verdict

Request changes — one narrow fix; the rest is sound and I'd merge with it addressed. Dormancy rightly lowers severity (no CI or default-path impact, no false-pass risk), but the workers>1 scheduler is not incidental dormant code — it is the deliverable. Merging preparation whose core new mechanic has a confirmed, cheaply fixable defect means the future canary starts on a known bug. The fix is a ~2-line guard plus one deterministic self-test; requiring it in-PR is the lower-total-cost path. F2–F4 are non-blocking.